Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support return the manager api's git hash and version #1408

Merged
merged 10 commits into from
Feb 4, 2021

Conversation

starsz
Copy link
Contributor

@starsz starsz commented Feb 1, 2021

Please answer these questions before submitting a pull request

  • Why submit this pull request?

  • [] Bugfix

  • New feature provided

  • Improve performance

  • Backport patches

  • Related issues

fix #1404

New feature or improvement

  • Describe the details and related test reports.

Add info API to support get the backend info.

@codecov-io
Copy link

codecov-io commented Feb 1, 2021

Codecov Report

Merging #1408 (b22303d) into master (94d952f) will increase coverage by 0.05%.
The diff coverage is 62.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1408      +/-   ##
==========================================
+ Coverage   45.15%   45.20%   +0.05%     
==========================================
  Files          35       37       +2     
  Lines        2516     2524       +8     
==========================================
+ Hits         1136     1141       +5     
- Misses       1215     1218       +3     
  Partials      165      165              
Impacted Files Coverage Δ
api/internal/utils/version.go 0.00% <0.00%> (ø)
api/internal/handler/tool/tool.go 71.42% <71.42%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 94d952f...b22303d. Read the comment docs.

Copy link
Member

@membphis membphis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use a better title, it confused me

api/internal/handler/tool/tool.go Outdated Show resolved Hide resolved
api/internal/handler/tool/tool.go Outdated Show resolved Hide resolved
@starsz starsz changed the title feat: support feat info (#1404) feat: support return the manager api's git hash and version (#1404) Feb 1, 2021
api/internal/handler/tool/tool.go Outdated Show resolved Hide resolved
api/internal/utils/utils.go Outdated Show resolved Hide resolved
api/internal/handler/tool/tool.go Outdated Show resolved Hide resolved
api/internal/handler/tool/tool.go Show resolved Hide resolved
api/test/e2e/info_test.go Outdated Show resolved Hide resolved
@starsz starsz changed the title feat: support return the manager api's git hash and version (#1404) feat: support return the manager api's git hash and version Feb 2, 2021
@juzhiyuan
Copy link
Member

@starsz try to sync with the latest codes from the master branch?

@starsz
Copy link
Contributor Author

starsz commented Feb 4, 2021

@starsz try to sync with the latest codes from the master branch?

It's already the latest. Please see the CI on the master branch.

@juzhiyuan
Copy link
Member

fine, then please request changes from other members, once the FE test is fixed, I will let you know.

@juzhiyuan
Copy link
Member

FE is working on why test failed, once test fixed, we will merge.

@starsz starsz reopened this Feb 4, 2021
@tokers tokers merged commit 597b046 into apache:master Feb 4, 2021
}

func (h *Handler) ApplyRoute(r *gin.Engine) {
r.GET("/version", wgin.Wraps(h.Version))
Copy link
Member

@juzhiyuan juzhiyuan Feb 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@starsz @nic-chen

This will lead to 2 kinds of APIs:

  1. /apisix/admin/xxx
  2. /version

When we deprecate admin-api in V3, we will have to omit that prefix apisix/admin either, to have ManagerAPIs.

Now FE meets one issue: because HTTP call is handled by UmiJS, which only supports one prefix, /apisix/admin or /, then FE has to make hack codes to be compatible with those 2 kinds of APIs.

I know it's wired to have something like /apisix/admin/version, but could we add a prefix for this API first? with some description about this hacking case. In V3, we may omit that prefix from all APIs.

cc @LiteSun

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.Got it.
I will add the prefix and ignore this API's auth checking in the next PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if we need to omit the prefix, but unification is appropriate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

We can get the backend version and git hash at the front end.
7 participants